There is TheGameLogic->isLoadingSave()

It's still false here when I load a game. This function is also called from other places during loading (with objectXferLoad == false), so a single 'loading' flag is not enough.

After #2508 that process was inverted which leads issues with the map shroud (see issue description). Object::setShroudClearingRange would be called (as part of S&D increased vision) when it shouldn't.

Can you explain why this breaks the shroud? I wonder if something else needs fixing that avoids breaking the shroud if this function is called, such as refreshing some state or so.

In short, SnD triggers a vision bonus, but this data is already xferred and causes incorrect shroud reveal undo calls.

I'm not entirely sure if I've got the entire picture, but here are some details:

1. bugfix(energy): Don't increase power production for disabled power plants on save game load #2508 moved the call to Object::setOrRestoreTeam, so that Object::m_partitionLastLook is now xferred prior to the call.
2. Search and Destroy provides objects with vision bonus. This triggers a call to update the shroud:

Object::handleShroud() Line 4916
Object::handlePartitionCellMaintenance() Line 4906
Object::setShroudClearingRange(float newShroudClearingRange) Line 5320
localApplyBattlePlanBonusesToObject(Object * obj, void * userData) Line 3598
Player::applyBattlePlanBonusesForObject(Object * obj) Line 3635
Player::becomingTeamMember(Object * obj, bool yes, bool objectXferLoad) Line 1061
Object::setOrRestoreTeam(Team * team, bool restoring, bool objectXferLoad) Line 931
Object::xfer(Xfer * xfer) Line 4267

3. Previously Object::unlook would return early because m_partitionLastLook->isInvalid() or m_howFar == 0.0f was still true, because that data wasn't xferred yet. But after bugfix(energy): Don't increase power production for disabled power plants on save game load #2508 m_partitionLastLook is already xferred at this point, so there's a call to PartitionManager::queueUndoShroudReveal.

Just to illustrate my findings in the previous message, the following appears to work as well:

if( xfer->getXferMode() == XFER_LOAD )
{
	//
	const Real look = m_partitionLastLook->m_howFar;
	m_partitionLastLook->m_howFar = 0.0f; // set to zero so that `Object::unlook` returns early
	assert(m_partitionLastLook->isInvalid());
	//

	Team *team = TheTeamFactory->findTeamByID( teamID );
	if( team == nullptr )
	{
		DEBUG_CRASH(( "Object::xfer - Unable to load team" ));
		throw SC_INVALID_DATA;
	}
	const Bool restoring = true;
	setOrRestoreTeam( team, restoring, true );

	//
	m_partitionLastLook->m_howFar = look;
	//
}

Would it perhaps be possible to decouple the PartitionManager xfer and init ready states so that everything can be xferred at random order but the PartitionManager will not be accepting mutation until after the xfer?

I'm not sure what you mean by that.

My understanding from your write ups so far is that PartitionManager is loaded into good state by xfer, but is then set into broken state by a call to handlePartitionCellMaintenance. I do wonder why exactly handlePartitionCellMaintenance breaks its subsequent state and how it can be avoided that such a call breaks it, for example by not accepting any state mutation until after all xfer is completed.

Basically I think it is absurd that calling handlePartitionCellMaintenance will break it under some circumstances.

I don't think guarding the PartitionManager state in particular is the solution. I have re-enabled an assertion that should get triggered if that state was modified by the Object xferring, though.

Seems to me that the way to fix this is adding a guard as early as possible, long before it would reach to the PartitionManager state, which is what this PR does.

I started looking yesterday and handleShroud looks mysterious so far.

Observations made:

1. Replacing ThePartitionManager->queueUndoShroudReveal with ThePartitionManager->undoShroudReveal in Object::unlook makes it behave almost good. But the implications of non-queued Undo Shroud are unclear.

2. Calling resetPendingUndoShroudRevealQueue on xfer load at the begin of PartitionManager::xfer improves the situation, but still is buggy.

I think we need to get a better understanding of the relationship between the shroud reveal undo queue and the xfer.

I think this would be a good use case for LLM to drill into the PartitionManager to really understand why the undo shroud reveal queue corrupts the stats in combination with shroud xfer.

Why the pending-undo-shroud queue corrupts cell stats across an xfer

Traced against 674e953 (main). All line numbers are GeneralsMD.

1. The invariant the shroud counters depend on

PartitionCell::m_shroudLevel[p].m_currentShroud is a saturating counter, not a plain one:

• addLooker — cur = min(cur - 1, -1) (PartitionManager.cpp:1278). From 1 it jumps straight to -1.
• removeLooker — if cur == -1 then cur = min(activeShroudLevel, 1), else cur++ (PartitionManager.cpp:1309-1315).

getShroudStatusForPlayer (PartitionManager.cpp:1384): 1 = SHROUDED, 0 = FOGGED, anything else = CLEAR.

The saturation at the boundaries is what makes N balanced add/remove pairs return to the start. It is also what makes an unbalanced pair unrecoverable:

• one extra removeLooker on a looked-at cell: -1 → 1 — the cell goes fully black while a unit is standing there looking at it.
• one extra removeLooker on a shrouded cell: 1 → 2 — this trips the existing "Someone is RemoveLooker-ing on a cell that is not looked at. This will make a permanent shroud blob." assert, and 2 reads back as CLEAR.
• and 2 never heals: the next real addLooker does min(2-1, -1) = -1, the next removeLooker does -1 → 1. The offset is absorbed by the saturation and the cell is permanently biased.

So every doShroudReveal must be matched by exactly one undoShroudReveal over the same circle. That is the only invariant in play here.

2. Why an xfer can break it: two halves of one ledger, restored differently

Block order in GameState::init (SaveGame/GameState.cpp:312-323):

... CHUNK_TeamFactory, CHUNK_Players, CHUNK_GameLogic (← all Objects), ...
... CHUNK_InGameUI, CHUNK_Partition (← cells + pending queue), ...

Objects are restored seven chunks before the PartitionManager. And inside PartitionManager::xfer the two halves of the shroud ledger are restored by opposite mechanisms:

• cells — PartitionCell::xfer does xferUser(&m_shroudLevel, ...) (PartitionManager.cpp:1528). A blind overwrite. Anything done to the cells earlier in the load is silently discarded.
• queue — the saved SightingInfos are pushed onto the existing queue (PartitionManager.cpp:4746-4751). Anything queued earlier in the load survives, and sits in front of them.

That asymmetry is the whole bug. Any look/unlook pair generated between the start of object load and CHUNK_Partition has its add erased and its undo retained.

And note which of the four shroud ops is deferred:

Exactly one operation in the system can outlive the cell overwrite, and it is the one that decrements lookers. Everything else in handleShroud() is self-cancelling across the overwrite.

3. The concrete chain for S&D

Preconditions restored earlier in Object::xfer than the team call: position (Object.cpp:4148-4151), m_partitionLastLook (4219), m_shroudClearingRange (4245). setOrRestoreTeam is at 4277 — this is what #2508 moved.

1. setOrRestoreTeam(restoring=true) → becomingTeamMember(yes=true) → applyBattlePlanBonusesForObject.
2. localApplyBattlePlanBonusesToObject (Player.cpp:3593-3594):

   objectToModify->setVisionRange( obj->getVisionRange() * bonus->m_sightRangeScalar );
   objectToModify->setShroudClearingRange( obj->getShroudClearingRange() * bonus->m_sightRangeScalar );

   It multiplies the current value — which was already restored with the bonus baked in. So R → R·s.
3. setShroudClearingRange only does work if (newShroudClearingRange != m_shroudClearingRange) (Object.cpp:5304). The redundant re-application is precisely what defeats that guard. If the value were merely re-derived correctly it would compare equal and nothing would happen.
4. → handlePartitionCellMaintenance() → handleShroud():
   • unlook() — post-bugfix(energy): Don't increase power production for disabled power plants on save game load #2508 m_partitionLastLook is valid, so instead of returning early it queues an undo for circle(P, R, M) and resets m_partitionLastLook.
   • unshroud() / shroud() — immediate, self-cancelling, harmless.
   • look() — immediate doShroudReveal(P, R·s, M), and sets m_partitionLastLook = (P, R·s, M).
5. CHUNK_Partition overwrites every cell → the addLooker from step 4 is gone. The queued undo is not.
6. gameStatePostProcessLoad → ThePartitionManager->update() → the injected entry fires UnlookPersistDuration (30) frames later and applies removeLooker over circle(P, R, M) against counters that never received the matching add.

Two independent imbalances, not one

(a) the orphan undo — one unmatched removeLooker per S&D object, over its saved look circle.

(b) a radius mismatch that outlives it — after the overwrite, the cells hold the add for radius R (from the file) but the object's m_partitionLastLook claims radius R·s. The next time that object moves, unlook removes a larger circle than was ever added, leaving an unmatched-remove annulus of width R·s − R. This one is not in the PartitionManager at all; it is desync between object state and cell state.

This predicts both of your experiments:

• "replacing queueUndoShroudReveal with undoShroudReveal in unlook makes it behave almost good" — an immediate undo gets erased by the overwrite along with the look, so (a) disappears. (b) survives → "almost".
• "calling resetPendingUndoShroudRevealQueue on xfer load improves the situation, but still is buggy" — same thing: kills (a), leaves (b).

It also reproduces every symptom in #2882

• "wait a few seconds" — the injected entry is timestamped now + 30, so the damage lands ~1s after load, not at load.
• "the place where the unit was standing, in the radius of its vision" goes black — those cells were -1 (unit looking); the orphan remove takes -1 → 1 = SHROUDED.
• "move the unit and the original area resets, overlaps go darker/lighter" — imbalance (b) firing on the next move, plus 1 → 2 cells reading back as CLEAR.
• "send the unit where the S&D detection range doesn't reach" — needed because a second overlapping looker (the Strategy Center's own) would mask the off-by-one (-2 → -1, still CLEAR).
• "only happens on TSH, not retail" — retail xfers m_partitionLastLook after setOrRestoreTeam, so unlook() returns early on isInvalid() (Object.cpp:5152).

4. On "make the PartitionManager reject mutation until xfer completes"

The idea is sound and I'd support it as hardening, with two caveats:

It has to gate both halves. A flag checked only inside doShroudReveal/undoShroudReveal still lets queueUndoShroudReveal push. The gate belongs on all five entry points, or on Object::handleShroud itself.

It has to be armed before the first object is xferred, i.e. driven by the load, not by PartitionManager::xfer — by the time that chunk runs, the damage is already queued.

There is an existing flag with exactly the right scope, and it isn't the one suggested earlier in this thread: TheGameState->isInLoadGame() (Common/GameState.h:177). It's latched TRUE around the whole xferSaveData call (SaveGame/GameState.cpp:680), so it covers CHUNK_GameLogic and CHUNK_Partition. TheGameLogic->isLoadingSave() is a different flag, set/cleared around map load in GameStateMap.cpp:253/447 — which is why it read false when tested here.

But hardening the PartitionManager cannot fix this bug, and I think that's the load-bearing point. Imbalance (b) — and the permanent inflation of m_visionRange / m_shroudClearingRange described below — happen entirely outside the PartitionManager. A PartitionManager that refuses mutation during xfer would emerge from the load with clean cells and an object whose vision range is R·s and whose m_partitionLastLook is stale. It would look fixed and then desync on the next move. The guard has to be upstream of the bonus application, which is what this PR does.

5. A second bug this PR fixes that hasn't been named

Step 2 above is a compounding error independent of the shroud. Every save/load with an active battle plan multiplies each affected object's m_visionRange and m_shroudClearingRange by m_sightRangeScalar again — s² after one load, s³ after two.

Turning the plan off calls removeBattlePlanBonusesForObject, which divides by the scalar exactly once (Player.cpp:3641-3642), so N save/loads leave a permanent sᴺ inflation that no amount of toggling recovers. Worth calling out in the PR description: the early return fixes an unbounded stat drift, not just a rendering artefact.

6. On the re-enabled assert

The 2003 comment blamed "a setTeam call for each guy on a sub-team" — that is the Object::xfer → setOrRestoreTeam path this PR now guards, so removing the comment is right.

I grepped for other paths that could queue an unlook inside the same window: no setTeam/setTemporaryTeam call appears inside any ::xfer or ::loadPostProcess body in GeneralsMD/Code/GameEngine/Source. loadPostProcess runs after CHUNK_Partition anyway, so anything there is balanced against authoritative cells. The assert should be clean.

Caveat on method: this is a static read of the current tree, not an instrumented run. The two independent confirmations are that it predicts the outcome of both experiments above without being fitted to them, and that it accounts for each of the five separate symptoms in #2882 including the timing delay.

I strongly suspect the devs experienced the same issue at some point during development, though for the v1.04 code this assertion should not get triggered AFAICT.

I have removed the comment with the assumption that this assertion won't get triggered after this PR. If that's not true, the old comment could / should be restored again.

Do we need the third parameter on setOrRestoreTeam? Only Player::becomingTeamMember actually needs to know it's being called during a load - for setOrRestoreTeam, restoring always equals objectXferLoad right?

setOrRestoreTeam is also called from setTemporaryTeam / setTeam, which is called all over.

All of those paths go through setTemporaryTeam, which hardcodes restoring = false. So setOrRestoreTeam has two direct call sites, and the flags are equal at both - setTemporaryTeam passes (false, false), Object::xfer passes (true, true). Only xfer ever produces true, at least as of now. Anyway, I think it's more correct the way you have it, if someone adds a restoring caller later, we'd want the parameters this way.

That seems reasonable for now. Changed.

I wonder if objectXferLoad should be renamed now that it's just an alias for 'restoring'.

Yeah naming a parameter after who calls it rather than what it means bugs me. restoring works for me.